-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip #131405
Conversation
rustbot has assigned @albertlarsan68. Use |
We could make this same change to the illumos and AIX targets which also hardcode a path to a strip binary. Pinging relevant people for those targets to see if this would be desirable. @ecnelises @bzEq (listed as target maintainers for |
This comment was marked as resolved.
This comment was marked as resolved.
159f31c
to
eadf0dd
Compare
src/bootstrap/src/core/builder.rs
Outdated
@@ -1151,6 +1151,11 @@ impl<'a> Builder<'a> { | |||
self.ensure(compile::Sysroot::new(compiler)) | |||
} | |||
|
|||
/// Returns the bindir for a compiler's sysroot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably t-bootstrap will have opinions here, because there are 2 "bin" directories 😓
Thank you for posting this. I tried twice and couldn't even figure out where to start. |
AFAIK some change is needed for illumos too because using the default strip binary of Linux host causes a problem (oxidecomputer/helios#147). cc @sunshowers |
let mut new_path = sess.get_tools_search_paths(false); | ||
if let Some(path) = env::var_os("PATH") { | ||
new_path.extend(env::split_paths(&path)); | ||
} | ||
cmd.env("PATH", env::join_paths(new_path).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also: it's probably safer to look for rust-objcopy
in the tools search path and launch w/ the absolute path cmd rather than overriding the $PATH?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably is but we override the PATH
for rust-lld
so I'll keep it this way and we can change both together later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, do we? I remember we set the linker search path to the lld wrappers in the sysroot without overriding the PATH (so they should execute ../rust-lld
without overrides), and that rustup ensures the PATH contains the bin and rustlib/bin folders, but didn’t remember rustc overrode it for rust-lld.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cmd.env("PATH", env::join_paths(new_path).unwrap()); |
I believe this is where it happens
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's indeed likely it for the targets involving the linker directly. We don't need/make use of that on the common linux/mac targets.
Hey, with my apologies, someone pointed this out to me recently and I owe a PR to fix the docs and list some maintainers there.
When we're running on an illumos host and compiling for an illumos target, we definitely want to use the system strip binary explicitly. If we're cross compiling, either on an illumos host for a non-illumos target, or on a non-illumos host, I don't have a particular preference and we should do whatever is tested to work there. Does that make sense? |
Thanks for headsup. On AIX, we are still using |
…nnethercote Fix hardcoded strip path when cross-compiling from Linux to Darwin Fixes rust-lang#131206. I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression. `@rustbot` label O-apple
Rollup merge of rust-lang#131480 - madsmtm:macos-fix-strip-binary, r=nnethercote Fix hardcoded strip path when cross-compiling from Linux to Darwin Fixes rust-lang#131206. I fear that rust-lang#131405 might end up taking some time, so opening this PR to resolve the regression. `@rustbot` label O-apple
This comment was marked as resolved.
This comment was marked as resolved.
eadf0dd
to
d9b8a78
Compare
Okay, I've kept AIX's logic as it is, but I have added a warning when cross-compiling that whatever is in
That makes sense. I've kept |
d9b8a78
to
89cd82d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for the technical side in bootstrap
r? t-compiler for the compiler side of things |
This comment was marked as resolved.
This comment was marked as resolved.
89cd82d
to
a0b6f83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me (with compiler hat 🎩 on). Feel free to r=albertlarsan68 and me after a rebase.
/// Returns the bindir for a compiler's sysroot. | ||
pub fn sysroot_bindir(&self, compiler: Compiler, target: TargetSelection) -> PathBuf { | ||
self.sysroot_libdir(compiler, target).parent().unwrap().join("bin") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remark (for myself): send a follow-up to avoid this dance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up PR: #132723
@rustbot author |
a0b6f83
to
f745467
Compare
@bors r=jieyouxu,albertlarsan68 |
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#131261 (Stabilize `UnsafeCell::from_mut`) - rust-lang#131405 (bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip) - rust-lang#132077 (Add a new `wide-arithmetic` feature for WebAssembly) - rust-lang#132562 (Remove the `wasm32-wasi` target from rustc) - rust-lang#132660 (Remove unused errs.rs file) Failed merges: - rust-lang#131721 (Add new unstable feature `const_eq_ignore_ascii_case`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131405 - davidtwco:hardcoded-strip-macos, r=jieyouxu,albertlarsan68 bootstrap/codegen_ssa: ship llvm-strip and use it for -Cstrip Fixes rust-lang#131206. - Includes `llvm-strip` (a symlink to `llvm-objcopy`) in the compiler dist artifact so that it can be used for `-Cstrip` instead of the system tooling. - Uses `llvm-strip` instead of `/usr/bin/strip` for macOS. macOS needs a specific linker and the system one is preferred, hence rust-lang#130781 but that doesn't work when cross-compiling, so use the `llvm-strip` utility instead. cc rust-lang#123151
Only copy, rename and link `llvm-objcopy` if llvm tools are enabled Fixes rust-lang#132719. cc `@bjorn3` who reported the bootstrapping problem for cg_clif. cc `@davidtwco` in case this might be problematic for linux -> macOS cross-compile builds, but seems very unlikely. cc `@albertlarsan68` (co-reviewed rust-lang#131405) r? bootstrap
Only copy, rename and link `llvm-objcopy` if llvm tools are enabled Fixes rust-lang#132719. cc `@bjorn3` who reported the bootstrapping problem for cg_clif. cc `@davidtwco` in case this might be problematic for linux -> macOS cross-compile builds, but seems very unlikely. cc `@albertlarsan68` (co-reviewed rust-lang#131405) r? bootstrap
…ution, r=onur-ozkan Unify `sysroot_target_{bin,lib}dir` handling Follow-up to rust-lang#131405 (comment) where `sysroot_target_bindir` had to do some dancing because the sysroot ensure logic embedded in `sysroot_target_libdir` returned `$sysroot/$relative_lib/rustlib/$target/lib` and not the `rustlib` parent `$sysroot/$relative_lib/rustlib/`. This PR pulls out the sysroot ensure logic into a helper, and return `$sysroot/$relative_lib/rustlib/` instead so `sysroot_target_bindir` doesn't have to do parent traversal from the path returned from `sysroot_target_libdir`, and also make them easier to follow in that they are now clearly closely related based on the common target sysroot ensure logic.
Rollup merge of rust-lang#132723 - jieyouxu:sysroot-dance-dance-revolution, r=onur-ozkan Unify `sysroot_target_{bin,lib}dir` handling Follow-up to rust-lang#131405 (comment) where `sysroot_target_bindir` had to do some dancing because the sysroot ensure logic embedded in `sysroot_target_libdir` returned `$sysroot/$relative_lib/rustlib/$target/lib` and not the `rustlib` parent `$sysroot/$relative_lib/rustlib/`. This PR pulls out the sysroot ensure logic into a helper, and return `$sysroot/$relative_lib/rustlib/` instead so `sysroot_target_bindir` doesn't have to do parent traversal from the path returned from `sysroot_target_libdir`, and also make them easier to follow in that they are now clearly closely related based on the common target sysroot ensure logic.
Fixes #131206.
llvm-strip
(a symlink tollvm-objcopy
) in the compiler dist artifact so that it can be used for-Cstrip
instead of the system tooling.llvm-strip
instead of/usr/bin/strip
for macOS. macOS needs a specific linker and the system one is preferred, hence Fix up setting strip = true in Cargo.toml makes build scripts fail in… #130781 but that doesn't work when cross-compiling, so use thellvm-strip
utility instead.cc #123151